Skip to content

fix(cli): address 0.4.8 verification findings#218

Merged
ivan-flamingo merged 11 commits into
mainfrom
chore/cli-audit-remediation-3
Jul 13, 2026
Merged

fix(cli): address 0.4.8 verification findings#218
ivan-flamingo merged 11 commits into
mainfrom
chore/cli-audit-remediation-3

Conversation

@oleg-flamingocx

@oleg-flamingocx oleg-flamingocx commented Jul 13, 2026

Copy link
Copy Markdown
Contributor
  • Blocker: disable dex in the embedded ArgoCD values. The argo-cd chart 10.1.3 defaults dex.enabled=true, and dexidp/dex:v2.45.1's arm64 image intermittently SIGSEGVs under emulation on Apple Silicon before its first log line -> CrashLoopBackOff -> the 7m helm --wait never completes -> fresh installs fail. OpenFrame's login uses the local developer account, never dex. Verified with helm template: 0 dex-server objects with our values vs 3 at the chart default; core components unchanged.

  • Track stall per application instead of one global fingerprint. The old fingerprint covered the whole not-ready set, so a single neighbour oscillating Missing<->OutOfSync reset the 90s timer every tick while a genuinely stuck app sat Healthy+OutOfSync and never accrued stall time — the flagship N3 fix never fired in exactly the scenario it was written for. stallTracker times each app independently; a stuck app is now detected regardless of noisy neighbours.

  • Assert the deployed ref before declaring success. app install --ref <old-branch> writes the flattened repository.branch, but a branch whose chart predates that key ignores it — children render from main, everything goes Healthy+Synced, and the CLI printed "17/17 ready ... SUCCESS" for a deployment of the wrong ref. verifyRefPinning compares OSS-repo children's targetRevision against the requested ref and fails loudly on a mismatch; it skips default refs (main/master/HEAD), ignores foreign-repo children, and normalizes URLs with embedded credentials.

  • Map a pending helm release to an actionable hint. A release wedged in pending-* by an interrupted operation ("another operation is in progress") got the generic "wait and retry" timeout hint — but retrying hits the same pending release. Match it before the timeout case and point at helm rollback, noting pending releases need helm list -a to be seen.

  • Stop dumping raw helm output under --verbose. The full helm stdout, the helm list JSON, and the 150-line helm status inventory (chart NOTES twice) buried the useful lines; stderr warnings on the install path are still shown.

Summary by CodeRabbit

  • New Features
    • Added verification to detect Argo CD Git ref mismatches for child apps, with actionable mismatch messaging.
    • Improved stall detection with per-application tracking and a heartbeat indicator for long, quiet operations.
  • Bug Fixes
    • Disabled Argo CD Dex for local installs to avoid install failures.
    • Reduced Helm diagnostic noise and improved stuck-release guidance (suggesting rollback for pending Helm releases).
    • Hardened repo-server recovery with HARD refresh, plus refined app-of-apps child syncing using tracking markers.
  • Documentation
    • Added local development guidance for overriding Argo CD chart values.
  • Tests
    • Expanded coverage for ref pinning, HARD refresh, tracking selection, and stall behavior.
  • Chores
    • Improved help coverage in CI and refined release note generation.

- Blocker: disable dex in the embedded ArgoCD values. The argo-cd chart
10.1.3 defaults dex.enabled=true, and dexidp/dex:v2.45.1's arm64 image
intermittently SIGSEGVs under emulation on Apple Silicon before its first log
line -> CrashLoopBackOff -> the 7m `helm --wait` never completes -> fresh
installs fail. OpenFrame's login uses the local developer account, never dex.
Verified with `helm template`: 0 dex-server objects with our values vs 3 at
the chart default; core components unchanged.

- Track stall per application instead of one global fingerprint. The old
fingerprint covered the whole not-ready set, so a single neighbour oscillating
Missing<->OutOfSync reset the 90s timer every tick while a genuinely stuck app
sat Healthy+OutOfSync and never accrued stall time — the flagship N3 fix never
fired in exactly the scenario it was written for. stallTracker times each app
independently; a stuck app is now detected regardless of noisy neighbours.

- Assert the deployed ref before declaring success. `app install --ref
<old-branch>` writes the flattened repository.branch, but a branch whose chart
predates that key ignores it — children render from main, everything goes
Healthy+Synced, and the CLI printed "17/17 ready ... SUCCESS" for a deployment
of the wrong ref. verifyRefPinning compares OSS-repo children's targetRevision
against the requested ref and fails loudly on a mismatch; it skips default
refs (main/master/HEAD), ignores foreign-repo children, and normalizes URLs
with embedded credentials.

- Map a pending helm release to an actionable hint. A release wedged in
pending-* by an interrupted operation ("another operation is in progress")
got the generic "wait and retry" timeout hint — but retrying hits the same
pending release. Match it before the timeout case and point at `helm
rollback`, noting pending releases need `helm list -a` to be seen.

- Stop dumping raw helm output under --verbose. The full helm stdout, the
`helm list` JSON, and the 150-line `helm status` inventory (chart NOTES twice)
buried the useful lines; stderr warnings on the install path are still shown.
@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 34d04258-2f12-4a97-bc4e-f04349f77fa2

📥 Commits

Reviewing files that changed from the base of the PR and between 82806dc and b69861d.

📒 Files selected for processing (22)
  • .github/release.yml
  • .github/workflows/test.yml
  • .goreleaser.yml
  • Makefile
  • cmd/prerequisites/prerequisites.go
  • internal/bootstrap/service.go
  • internal/chart/providers/argocd/values.go
  • internal/chart/providers/argocd/values_merge_test.go
  • internal/chart/providers/argocd/wait.go
  • internal/chart/ui/operations_test.go
  • internal/chart/utils/types/configuration.go
  • internal/chart/utils/types/interfaces.go
  • internal/cluster/prerequisites/sets.go
  • internal/prerequisites/prerequisites.go
  • internal/prerequisites/runner.go
  • internal/prerequisites/runner_test.go
  • internal/shared/errors/errors_test.go
  • internal/shared/errors/friendly_upstream_test.go
  • internal/shared/ui/messages.go
  • internal/shared/ui/messages_test.go
  • internal/shared/wsllauncher/launcher_test.go
  • tests/integration/common/cluster_management.go
💤 Files with no reviewable changes (7)
  • internal/shared/ui/messages.go
  • internal/shared/ui/messages_test.go
  • internal/shared/wsllauncher/launcher_test.go
  • internal/chart/ui/operations_test.go
  • internal/shared/errors/errors_test.go
  • internal/bootstrap/service.go
  • internal/chart/utils/types/configuration.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • internal/chart/providers/argocd/wait.go
  • internal/chart/providers/argocd/values.go
  • internal/chart/providers/argocd/values_merge_test.go
  • internal/shared/errors/friendly_upstream_test.go

📝 Walkthrough

Walkthrough

Argo CD now tracks stalls per application, supports annotation-based ownership, hard-refreshes recovered applications, validates ref pinning, and merges user chart overrides. Dex is disabled by default, Helm operations emit heartbeats, prerequisite errors include specific reasons, and project tooling is updated.

Changes

Argo CD runtime validation

Layer / File(s) Summary
Per-application stall handling
internal/chart/providers/argocd/stall.go, internal/chart/providers/argocd/stall_test.go, internal/chart/providers/argocd/wait.go
Tracks unchanged health and sync state independently per application and handles stalled stragglers in the wait loop.
Resource tracking and recovery refresh
internal/chart/providers/argocd/sync.go, internal/chart/providers/argocd/diagnostics.go, internal/chart/providers/argocd/hardrefresh_test.go, internal/chart/providers/argocd/sync_tracking_test.go, internal/chart/providers/argocd/assess.go, internal/chart/providers/argocd/wait.go
Selects children through labels or tracking annotations and applies hard refreshes after repo-server recovery.
App-of-Apps ref validation
internal/chart/providers/argocd/refassert.go, internal/chart/providers/argocd/refassert_test.go, internal/chart/providers/argocd/wait.go
Normalizes refs and repository URLs, reports mismatches, and validates pinning after stabilization.

Argo CD values and Helm operations

Layer / File(s) Summary
Values merge and Dex baseline
internal/chart/providers/argocd/values.go, internal/chart/providers/argocd/values_merge_test.go, internal/chart/providers/argocd/argocd-values.yaml, internal/chart/providers/argocd/values_test.go, docs/development/setup/local-development.md
Adds deep-merged argocd: overrides, explicitly disables Dex, and documents and tests the values behavior.
Helm value wiring and heartbeat
internal/chart/providers/helm/manager.go, internal/shared/ui/spinner/heartbeat.go, internal/shared/ui/spinner/heartbeat_test.go
Feeds merged values into Helm installation and reports progress during blocking operations.
Helm diagnostics
internal/chart/providers/helm/argocd_wait.go, internal/chart/providers/helm/manager.go, internal/shared/errors/friendly.go, internal/shared/errors/friendly_upstream_test.go
Suppresses verification output and adds rollback-oriented hints for pending releases.

Prerequisite and project operations

Layer / File(s) Summary
Prerequisite reason propagation
internal/prerequisites/..., cmd/prerequisites/prerequisites.go, internal/cluster/prerequisites/sets.go
Propagates detailed prerequisite failure reasons and renders Docker daemon guidance distinctly.
Build, release, and CI updates
Makefile, .github/release.yml, .github/workflows/test.yml, .goreleaser.yml, tests/integration/common/cluster_management.go, internal/bootstrap/service.go
Updates build targets, release-note configuration, exhaustive help checks, cluster cleanup, integration command setup, and chart-install request wiring.
Supporting coverage and formatting
internal/chart/ui/operations_test.go, internal/chart/utils/types/*, internal/shared/errors/errors_test.go, internal/shared/ui/messages*, internal/shared/wsllauncher/launcher_test.go
Adds empty-input coverage and applies non-functional formatting adjustments.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ArgoCDWaitLoop
  participant ArgoCDApplications
  participant stallTracker
  participant RepoServer
  ArgoCDWaitLoop->>ArgoCDApplications: read application states
  ArgoCDWaitLoop->>stallTracker: observe per-application states
  stallTracker-->>ArgoCDWaitLoop: return stalled applications
  ArgoCDWaitLoop->>RepoServer: restart repo-server for recovery
  RepoServer-->>ArgoCDWaitLoop: report recovery
  ArgoCDWaitLoop->>ArgoCDApplications: apply hard refresh
Loading

Possibly related PRs

Suggested reviewers: ivan-flamingo, yaroslavmokflmg

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is clearly related to the PR and concisely summarizes the main goal: addressing CLI 0.4.8 verification findings.
Docstring Coverage ✅ Passed Docstring coverage is 95.24% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/cli-audit-remediation-3

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/chart/providers/argocd/wait.go (1)

528-554: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Ref-mismatch failure is reported through a spinner that already looks like a neutral stop, not a failure.

The spinner is unconditionally Stop()'d (Lines 536-540) before the ref-pinning check runs. If verifyRefPinning finds a mismatch, the function returns an error, but the spinner was already left in a neutral "stopped" state rather than the Fail() state used by every other failure path in this function (e.g., the timeout branch at Line 229). This directly undercuts the PR's stated intent of making a ref mismatch a "loud, actionable failure."

🐛 Proposed fix
 				if consecutiveAllReady >= stabilizationChecks {
-					spinnerMutex.Lock()
-					if !spinnerStopped && spinner != nil {
-						spinner.Stop()
-						spinnerStopped = true
-					}
-					spinnerMutex.Unlock()
-
 					// Everything is Healthy+Synced — but "ready" is not "correct".
 					// If a ref was requested, confirm ArgoCD is actually tracking it
 					// before declaring success; a legacy branch's chart silently
 					// deploys main and this is the only place that catches it (V3).
 					if config.AppOfApps != nil {
 						if mm := verifyRefPinning(apps, config.AppOfApps.GitHubRepo, config.AppOfApps.GitHubBranch); len(mm) > 0 {
+							spinnerMutex.Lock()
+							if !spinnerStopped && spinner != nil {
+								spinner.Fail("Deployed ref does not match the requested ref")
+								spinnerStopped = true
+							}
+							spinnerMutex.Unlock()
 							return refMismatchError(config.AppOfApps.GitHubBranch, mm)
 						}
 					}
 
+					spinnerMutex.Lock()
+					if !spinnerStopped && spinner != nil {
+						spinner.Stop()
+						spinnerStopped = true
+					}
+					spinnerMutex.Unlock()
+
 					pterm.Success.Println("All ArgoCD applications installed")
 					return nil
 				}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/chart/providers/argocd/wait.go` around lines 528 - 554, Update the
stabilization-success path in the wait function so ref-pinning validation occurs
before stopping the spinner, or otherwise ensure a mismatch calls the spinner’s
failure state before returning refMismatchError. Preserve the existing neutral
Stop behavior only for successful completion, matching the failure handling used
by the function’s timeout path.
🧹 Nitpick comments (1)
internal/shared/errors/friendly_upstream_test.go (1)

89-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add a pending-rollback test case.

The implementation matches "pending-rollback" but no test case exercises it. Adding one ensures all three pending-state patterns are covered.

✨ Add pending-rollback case
 	cases := []string{
 		`Error: UPGRADE FAILED: another operation (install/upgrade/rollback) is in progress`,
 		`Error: release app-of-apps failed, status: pending-upgrade`,
 		`cannot patch: release in pending-install state`,
+		`Error: release app-of-apps failed, status: pending-rollback`,
 	}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/shared/errors/friendly_upstream_test.go` around lines 89 - 109, Add
a pending-rollback error message to the cases table in
TestFriendlyHint_PendingReleaseSuggestsRollback, preserving the existing
assertions that the hint suggests rollback and excludes the generic timeout
guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/chart/providers/argocd/wait.go`:
- Around line 369-391: Update the stall handling around stall.observe and
stall.stalledStragglers so the hint branch is selected based on
config.SyncStragglersOnStall, not stragglerSyncTriggered; when sync is enabled,
never print the “will not sync on their own” hint after the one-time sync
attempt. Capture time.Now() once in a local now value and pass it to both stall
calls for a consistent tick.

---

Outside diff comments:
In `@internal/chart/providers/argocd/wait.go`:
- Around line 528-554: Update the stabilization-success path in the wait
function so ref-pinning validation occurs before stopping the spinner, or
otherwise ensure a mismatch calls the spinner’s failure state before returning
refMismatchError. Preserve the existing neutral Stop behavior only for
successful completion, matching the failure handling used by the function’s
timeout path.

---

Nitpick comments:
In `@internal/shared/errors/friendly_upstream_test.go`:
- Around line 89-109: Add a pending-rollback error message to the cases table in
TestFriendlyHint_PendingReleaseSuggestsRollback, preserving the existing
assertions that the hint suggests rollback and excludes the generic timeout
guidance.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b5e20f03-d51f-4d9d-8c5e-360af5baa1d2

📥 Commits

Reviewing files that changed from the base of the PR and between d75146d and 3747657.

📒 Files selected for processing (11)
  • internal/chart/providers/argocd/argocd-values.yaml
  • internal/chart/providers/argocd/refassert.go
  • internal/chart/providers/argocd/refassert_test.go
  • internal/chart/providers/argocd/stall.go
  • internal/chart/providers/argocd/stall_test.go
  • internal/chart/providers/argocd/values_test.go
  • internal/chart/providers/argocd/wait.go
  • internal/chart/providers/helm/argocd_wait.go
  • internal/chart/providers/helm/manager.go
  • internal/shared/errors/friendly.go
  • internal/shared/errors/friendly_upstream_test.go

Comment thread internal/chart/providers/argocd/wait.go
Both the ArgoCD (7m) and app-of-apps helm installs block on `helm --wait`
with no output while they run. On a TTY the spinner animates, but in a
non-interactive / CI / piped session there is no spinner, so the terminal sat
silent for minutes between "Installing..." and the result — and the 0.4.8
verification found users assume a hang and kill the process before the
diagnostics ever print, defeating the diagnostic work itself.

Add a Heartbeat primitive (ui/spinner/heartbeat.go): the non-interactive
counterpart to the animated Spinner. It emits "<label> (<elapsed>)" to stderr
every 30s while a long output-less call runs, is a no-op under --silent
("errors only"), writes to stderr so stdout stays clean, and Stop() joins its
goroutine for race-free teardown (verified with -race).

Wire it around both blocking installs, gated on spinner == nil (exactly the
non-interactive branch), scoped to the call via defer.
Repo-server recovery forced a "normal" application refresh, which only
re-compares against the manifest cache — exactly what a just-restarted
repo-server has lost. Apps that were Unknown because manifest generation
failed stayed Unknown until the wait timed out. It also refreshed only the
one app that tripped the detector, though a repo-server outage stalls every
app that couldn't generate manifests.

Add hardRefreshApplications, which annotates each named Application with
argocd.argoproj.io/refresh: hard (reusing refreshHardPatch) to force a git
re-fetch past the cache. triggerRepoServerRecovery now delegates to it for
the trigger app, and the wait loop hard-refreshes every currently-Unknown app
after a successful restart.

Tests drive the fake dynamic client and assert the patch is hard, not normal;
empty names are skipped; a nil dynamic client is a safe no-op.
…ust the label

`app upgrade --sync` selected the root's children by the label
app.kubernetes.io/instance=argocd-apps only. ArgoCD's "annotation" and
"annotation+label" resource-tracking methods leave that label empty and
record ownership in the argocd.argoproj.io/tracking-id annotation instead —
the case the verification run hit, where the primary selector matched nothing
and the fallback synced EVERY Application in the namespace. On a shared
cluster that also touches third-party Applications the CLI does not own.

trackingOwner now reads both markers: the label wins when set, otherwise the
annotation's owner (the segment before the first ":", split rather than
prefix-matched so "argocd-apps-foo" is not mistaken for "argocd-apps"). The
"sync everything" fallback is kept, but only as a genuine last resort when
neither marker is present anywhere.

Tests: the pure trackingOwner selection (owner parsing, look-alike guard),
annotation-only selection excluding a foreign owner without hitting the
fallback, and the fallback preserved when nothing is tracked. Verified to
fail on the old label-only path (it synced the foreign app).
The ArgoCD install used a baseline embedded in the binary with no user lever:
openframe-helm-values.yaml reached only the app-of-apps chart, so a user could
not change an ArgoCD value (e.g. re-enable dex) without rebuilding the CLI.

Add a dedicated top-level `argocd:` section to that file, deep-merged over the
embedded baseline (maps merge, scalars/lists replace — Helm semantics) and fed
to helm via stdin. Only that subtree is used, never the whole file: the rest
targets the app-of-apps chart under a different (flat) schema and carries the
docker registry password, which must not leak into the argo-cd release. The CLI
warns and lists the overridden keys, since a bad override can break the install;
with no `argocd:` section the baseline is returned unchanged.

Verified with helm template that `argocd: {dex: {enabled: true}}` re-enables
dex-server while the baseline (developer account, RBAC) survives the merge.
Tests cover the no-op, re-enable, deep-merge/list-replace, and key-sorting
paths plus deepMerge directly. Documented in local-development.md and the
values.yaml header.
clean matched a broad `openframe-*` glob that also caught tracked files like
openframe-helm-values.example.yaml and deleted them; scope it to the
platform-suffixed binaries (openframe-{linux,darwin,windows}-*).

build-all claimed "all platforms" but built 3 of the 6 release targets,
missing linux/arm64, darwin/amd64, and windows/arm64 — so a cross-compile
break on those slipped past a pre-release `make build-all`. Build all six, to
match .goreleaser.yml.

test-integration's comment claimed it needs `make build`; the harness builds
its own binary (tests/integration/common/cli_runner.go), so correct it to the
real prerequisites (docker + k3d).

Add `all` to .PHONY. Extract the duplicated unit-test package list into
UNIT_PKGS. Auto-generate `help` from `## ` target comments so it can't drift.
Add fmt/vet/tidy targets so local gating matches CI (gofmt, go vet, go mod
tidy -diff). Add -trimpath to builds for reproducibility, matching the release.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/chart/providers/argocd/sync.go (1)

167-168: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Fallback warning message only mentions the label, not the annotation.

The code now checks both trackingInstanceLabel and trackingIDAnnotation via trackingOwner, but the fallback warning says "No applications carry the app.kubernetes.io/instance=argocd-apps tracking label" — omitting the annotation marker entirely. A user with annotation-based tracking (no label) who hits this fallback would be misled into thinking they need to add labels, when the real issue is that no annotation owner matches AppOfAppsName either.

🔧 Proposed fix
 		if len(children) > 0 {
-			pterm.Warning.Printf("No applications carry the %s=%s tracking label; syncing all %d applications in %q\n",
-				trackingInstanceLabel, AppOfAppsName, len(children), ArgoCDNamespace)
+			pterm.Warning.Printf("No applications carry the %s=%s tracking label or %s annotation; syncing all %d applications in %q\n",
+				trackingInstanceLabel, AppOfAppsName, trackingIDAnnotation, len(children), ArgoCDNamespace)
 		}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/chart/providers/argocd/sync.go` around lines 167 - 168, Update the
fallback warning in the sync flow to mention both tracking mechanisms checked by
trackingOwner: the tracking label and trackingIDAnnotation. Make the message
accurately state that no application has a matching label or annotation owner
before syncing all applications.
🧹 Nitpick comments (1)
internal/chart/providers/helm/manager.go (1)

420-431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate heartbeat-wrapping pattern; consider a shared helper.

Both blocking Helm calls use the same if spinner == nil { hb := uispinner.StartHeartbeat(...); defer hb.Stop() } closure pattern. A small helper would remove the duplication and keep the two call sites in sync if the wrapping logic ever changes.

♻️ Suggested helper
// runWithHeartbeat runs fn, emitting a heartbeat message on stderr only when
// there is no animated spinner (non-interactive/CI), to keep a `helm --wait`
// blocking call from looking hung.
func runWithHeartbeat(spinner *uispinner.Spinner, label string, fn func() (*executor.CommandResult, error)) (*executor.CommandResult, error) {
	if spinner == nil {
		hb := uispinner.StartHeartbeat(label, 0)
		defer hb.Stop()
	}
	return fn()
}
-	result, err := func() (*executor.CommandResult, error) {
-		if spinner == nil {
-			hb := uispinner.StartHeartbeat("Still installing ArgoCD (helm --wait, up to 7m)...", 0)
-			defer hb.Stop()
-		}
-		return h.installArgoCDHelm(ctx, config)
-	}()
+	result, err := runWithHeartbeat(spinner, "Still installing ArgoCD (helm --wait, up to 7m)...", func() (*executor.CommandResult, error) {
+		return h.installArgoCDHelm(ctx, config)
+	})

Also applies to: 653-667

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/chart/providers/helm/manager.go` around lines 420 - 431, Extract the
duplicated spinner-nil heartbeat wrapper used around both blocking Helm calls
into a shared helper near the relevant manager functions, accepting the spinner,
heartbeat label, and command callback. Update the ArgoCD installation flow and
the other referenced Helm call to use this helper while preserving each call’s
existing label, result, error behavior, and heartbeat cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/chart/providers/argocd/values.go`:
- Around line 46-56: Update MergedArgoCDValues to distinguish a missing
UserArgoCDKey from a present value with the wrong type: return the baseline
unchanged only when the key is absent, and return a descriptive error when the
key exists but is not a map. Preserve the existing empty-map behavior and
deepMerge flow for valid overrides so installArgoCDHelm can surface malformed
configurations.

---

Outside diff comments:
In `@internal/chart/providers/argocd/sync.go`:
- Around line 167-168: Update the fallback warning in the sync flow to mention
both tracking mechanisms checked by trackingOwner: the tracking label and
trackingIDAnnotation. Make the message accurately state that no application has
a matching label or annotation owner before syncing all applications.

---

Nitpick comments:
In `@internal/chart/providers/helm/manager.go`:
- Around line 420-431: Extract the duplicated spinner-nil heartbeat wrapper used
around both blocking Helm calls into a shared helper near the relevant manager
functions, accepting the spinner, heartbeat label, and command callback. Update
the ArgoCD installation flow and the other referenced Helm call to use this
helper while preserving each call’s existing label, result, error behavior, and
heartbeat cleanup.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 562f44c9-20a5-4ea2-9d58-3890a67ec820

📥 Commits

Reviewing files that changed from the base of the PR and between 3747657 and 82806dc.

📒 Files selected for processing (13)
  • docs/development/setup/local-development.md
  • internal/chart/providers/argocd/argocd-values.yaml
  • internal/chart/providers/argocd/assess.go
  • internal/chart/providers/argocd/diagnostics.go
  • internal/chart/providers/argocd/hardrefresh_test.go
  • internal/chart/providers/argocd/sync.go
  • internal/chart/providers/argocd/sync_tracking_test.go
  • internal/chart/providers/argocd/values.go
  • internal/chart/providers/argocd/values_merge_test.go
  • internal/chart/providers/argocd/wait.go
  • internal/chart/providers/helm/manager.go
  • internal/shared/ui/spinner/heartbeat.go
  • internal/shared/ui/spinner/heartbeat_test.go

Comment thread internal/chart/providers/argocd/values.go
Add exhaustive, drift-proof coverage of the CLI's non-interactive surface in
test.yml, complementing the hermetic cmd/help_matrix_test.go (which runs the
command tree in-process, not the built binary):

- "CLI: every command --help (exhaustive)": a recursive walker discovers the
  whole command tree from the built binary's help output and asserts every
  command exits 0 with a Usage section. Dynamic discovery means a new command
  is covered with no hand-maintained list; pure and non-interactive, so it runs
  on every OS (and on Windows exercises the WSL forward). Replaces the handful
  of hand-listed `X --help` lines in the version&help step.

- "Cluster: cleanup (releases + crictl image prune)": runs `cluster cleanup
  --force` on the live, populated cluster before teardown — the one first-class
  non-interactive command with no real-binary coverage, exercising the
  kube-context-pinned Helm uninstalls, ArgoCD finalizer stripping, and crictl
  node-image prune (k3d nodes run containerd, not docker).
….yml

GoReleaser's changelog is `use: github-native`, which delegates note
generation to GitHub's API and IGNORES GoReleaser's own groups/filters — so
the Features/Bug-fixes grouping and the docs/chore exclusions in .goreleaser.yml
never actually applied. With this repo's squash-merge and free-form PR titles
(0 of the last 30 commits match feat:/fix:, and the heavily-used `refactor:`
was in the exclude list), commit-title grouping would have dropped or misfiled
real changes anyway.

Add .github/release.yml so GitHub categorizes notes by PR LABEL — robust to
free-form titles — into Features (enhancement), Bug Fixes (bug), Documentation
(documentation), and a catch-all Other Changes, excluding automated/bot and
non-shipping PRs. Remove the now-dead groups/filters from .goreleaser.yml and
point a comment at the new file.

Effective next release; requires PRs to be labeled for categorization
(unlabeled PRs fall through to Other Changes, never silently dropped).
…on ref mismatch

Two wait-loop fixes plus a test gap, from review.

Stall handling chose the hint-vs-sync branch by whether the one-shot sync had
already fired (!stragglerSyncTriggered), not by mode. On the upgrade path
(SyncStragglersOnStall) that meant: fire the sync once, then on every later
stall tick fall through and print "they will not sync on their own — run
`openframe app upgrade --sync`" — advising the user to run the very path they
are already on. Select the branch by config.SyncStragglersOnStall so the hint
is confined to the non-sync (install) path. Capture time.Now() once for both
stall calls.

Ref-pinning validation stopped the spinner neutrally and then returned an
error, unlike the timeout path which Fails it. Decide the spinner's final state
from the outcome: Fail on a mismatch, neutral Stop on success.

Add a pending-rollback case to the friendly-hint test — the code already maps
it, the test didn't cover it.
…talled"

The prerequisites framework only knew IsSatisfied (a bool), so the renderer
printed "✗ <name> is not installed" for every unsatisfied item. Docker's
predicate is IsDockerRunning, so a Docker that is installed but whose daemon is
down was reported as "not installed" with "How to install" docs — both wrong;
the user needs to START it, not install it.

Add an optional Prerequisite.Detail that explains the specific reason when it
beats "not installed"; the runner carries it into MissingItem.Reason at every
missing-item site (including the installed-but-still-unsatisfied path, i.e. the
WSL-Alpine case where apk installs docker but no OpenRC starts the daemon). The
renderer shows the reason and drops the misleading install docs when the tool
is present; genuinely-absent tools are unchanged. Docker supplies the reason,
distinguishing a present binary (daemon down) from a truly absent one.
…g it

MergedArgoCDValues type-asserted userValues["argocd"] to a map and treated a
failed assertion the same as "key absent" — returning the baseline silently.
So a present-but-non-map override (a scalar, a list, or wrong indentation that
makes `argocd:` null with keys floating to the top level) was dropped with no
error and no warning, contradicting the "overrides are announced" UX and
matching the V3 silent-failure class.

Distinguish the cases: absent, bare `argocd:` (null), and `argocd: {}` stay a
no-op (baseline unchanged); a present non-map value returns a descriptive error
naming the key, which installArgoCDHelm surfaces so the install fails loudly.
Valid maps deep-merge as before.

Tests cover the malformed types (bool/string/list/number) erroring and the
null/empty no-op paths.
@ivan-flamingo ivan-flamingo enabled auto-merge (squash) July 13, 2026 12:49
@ivan-flamingo ivan-flamingo merged commit 3c042e0 into main Jul 13, 2026
9 checks passed
@ivan-flamingo ivan-flamingo deleted the chore/cli-audit-remediation-3 branch July 13, 2026 12:53
@oleg-flamingocx oleg-flamingocx self-assigned this Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants